chore: replace Pyright with ty #2115
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
b03158d to
a1f9cbc
Compare
a60a6c2 to
b425610
Compare
Greptile SummaryThis PR replaces Pyright with
|
| Filename | Overview |
|---|---|
| .pre-commit-config.yaml | Replaces pyright hook with ty check; adds always_run: true and require_serial: true, drops types: [python] (correct since pass_filenames: false renders it irrelevant). |
| pyproject.toml | Switches typing dep-group from pyright to ty==0.0.56; migrates [tool.pyright] to [tool.ty.src], [tool.ty.analysis] (with allowed-unresolved-imports), and [tool.ty.overrides]. |
| nemoguardrails/streaming.py | Introduces _EndOfStream named class to replace object() sentinel, enabling isinstance-based narrowing; push_chunk and _process type signatures updated accordingly; all sentinel checks preserved as identity (is) tests. |
| nemoguardrails/actions/action_dispatcher.py | Adds AsyncInvokableAction, RunnableAction Protocols and RegisteredAction TypeAlias; replaces cast(Callable, fn) with typed walrus-operator extraction for ainvoke; raises ValueError for actions without __name__. |
| nemoguardrails/rails/llm/llmrails.py | Guards reasoning-trace prepend and prompt-mode response construction with isinstance(message_content, str), preventing silent TypeError when content is an exception dict; uses # ty: ignore for two genuinely unresolvable dict-access diagnostics. |
| nemoguardrails/kb/kb.py | Passes explicit threshold=None to self.index.search(); BasicEmbeddingsIndex.search and the abstract base already accept this parameter, so no compatibility concern. |
| nemoguardrails/llm/taskmanager.py | Widens parse_task_output task parameter to Union[str, Task] (matching actual callers); adds ValueError guard in register_filter for callables without __name__. |
| nemoguardrails/server/api.py | Rewrites optional chainlit import to use else clause of try/except, letting ty understand the type of mount_chainlit in each branch without a suppression comment. |
| .github/workflows/lint.yml | Changes uv run make pre-commit to make pre-commit; the make target now internally calls uv run --locked --group=dev, so CI still runs inside the venv. |
| tests/test_guardrail_exceptions.py | Adds two tests covering the new isinstance(message_content, str) branches: exception payload with generation options yields GenerationResponse, and reasoning trace is not prepended to a non-string content dict. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["push_chunk(chunk)"] --> B{chunk is None?}
B -- Yes --> C[chunk = END_OF_STREAM]
B -- No --> D{chunk is END_OF_STREAM?}
D -- Yes --> E[pass – already sentinel]
D -- No --> F{isinstance chunk, str?}
F -- Yes --> G[valid string chunk]
F -- No --> H[invalid type – ignored]
C --> I["_process(END_OF_STREAM)"]
E --> I
G --> J["_process(chunk: str)"]
I --> K{enable_buffer?}
J --> K
K -- Yes, str chunk --> L[buffer += chunk]
K -- Yes, END_OF_STREAM --> M[noop / fall through]
K -- No, str chunk --> N[completion += chunk, check stop tokens]
K -- No, END_OF_STREAM --> O{pipe_to set?}
N --> O
O -- Yes --> P[asyncio.create_task pipe_to.push_chunk]
O -- No, include_metadata --> Q[queue.put chunk_dict]
O -- No, plain --> R[queue.put chunk]
Q --> S{chunk is END_OF_STREAM?}
R --> S
S -- Yes --> T[streaming_finished_event.set]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["push_chunk(chunk)"] --> B{chunk is None?}
B -- Yes --> C[chunk = END_OF_STREAM]
B -- No --> D{chunk is END_OF_STREAM?}
D -- Yes --> E[pass – already sentinel]
D -- No --> F{isinstance chunk, str?}
F -- Yes --> G[valid string chunk]
F -- No --> H[invalid type – ignored]
C --> I["_process(END_OF_STREAM)"]
E --> I
G --> J["_process(chunk: str)"]
I --> K{enable_buffer?}
J --> K
K -- Yes, str chunk --> L[buffer += chunk]
K -- Yes, END_OF_STREAM --> M[noop / fall through]
K -- No, str chunk --> N[completion += chunk, check stop tokens]
K -- No, END_OF_STREAM --> O{pipe_to set?}
N --> O
O -- Yes --> P[asyncio.create_task pipe_to.push_chunk]
O -- No, include_metadata --> Q[queue.put chunk_dict]
O -- No, plain --> R[queue.put chunk]
Q --> S{chunk is END_OF_STREAM?}
R --> S
S -- Yes --> T[streaming_finished_event.set]
Reviews (10): Last reviewed commit: "update" | Re-trigger Greptile
📝 WalkthroughWalkthroughMigrates the project's static type checker from Pyright to ty across pre-commit config, pyproject.toml, and documentation. Removes numerous ChangesPyright to ty migration
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
AGENTS.md (1)
98-98: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStale "Pyright" reference after the ty migration.
Line 98 still says "Standalone Ruff, Ruff format, and Pyright runs are local diagnosis only" even though the Validation table above (line 86) and the pre-commit/pyproject config now use
ty. This is inconsistent with the rest of the migration.📝 Proposed fix
-- Standalone Ruff, Ruff format, and Pyright runs are local diagnosis only; run +- Standalone Ruff, Ruff format, and ty runs are local diagnosis only; run pre-commit on changed files before handoff and report if it is skipped.As per coding guidelines,
**/*.mdfiles should be updated when changing configuration syntax or tooling behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` at line 98, Update the stale tooling guidance in AGENTS.md so it matches the ty migration: replace the Pyright reference in the local diagnosis note with ty, keeping the wording consistent with the Validation table and the pre-commit/pyproject configuration. Locate the sentence near the standalone Ruff/Ruff format/pyright guidance and revise it to refer to Ruff, Ruff format, and ty instead.Source: Coding guidelines
🧹 Nitpick comments (2)
nemoguardrails/actions/action_dispatcher.py (2)
120-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring still says
Callable; param is nowAny.Minor staleness after the signature change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nemoguardrails/actions/action_dispatcher.py` around lines 120 - 134, The docstring for register_action is stale after the signature change from Callable to Any. Update the Args description for action in action_dispatcher.py so it matches the current register_action(action: Any, name: Optional[str] = None, override: bool = True) signature and reflects that action may be a broader object, while keeping the rest of the behavior and action_meta/__name__ lookup in sync.
128-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName-validation logic duplicated across three files.
The
name or getattr(x, "__name__", None)+isinstance(..., str)/ValueErrorpattern here is repeated almost verbatim innemoguardrails/actions/actions.py(action decorator) andnemoguardrails/llm/taskmanager.py(register_filter). Consider extracting a small shared helper (e.g.resolve_explicit_name(name, obj, kind)) to avoid triple maintenance of the same validation and error message.♻️ Example shared helper
def _require_explicit_name(name: Optional[str], obj: Any, kind: str) -> str: resolved = name or getattr(obj, "__name__", None) if not isinstance(resolved, str) or not resolved: raise ValueError(f"An explicit name is required for {kind} without __name__.") return resolved🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nemoguardrails/actions/action_dispatcher.py` around lines 128 - 134, The name resolution and validation logic is duplicated in the action dispatcher path, the action decorator, and register_filter, so extract a shared helper for resolving explicit names and enforcing the string/non-empty check. Update the logic in action_dispatcher, actions, and taskmanager to call the shared helper instead of repeating the `name or getattr(..., "__name__", None)` plus `isinstance(..., str)`/`ValueError` pattern, and keep the error message consistent via the helper’s kind parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nemoguardrails/embeddings/providers/openai.py`:
- Around line 49-53: The OpenAI provider import flow currently imports OpenAI
before checking the installed openai version, so 0.x installs fail before the
unsupported-version guard can run. Update the import/validation sequence in the
openai provider module so the version check happens before accessing OpenAI, and
preserve the original ImportError context by re-raising with either from err or
from None in the import handling path.
---
Outside diff comments:
In `@AGENTS.md`:
- Line 98: Update the stale tooling guidance in AGENTS.md so it matches the ty
migration: replace the Pyright reference in the local diagnosis note with ty,
keeping the wording consistent with the Validation table and the
pre-commit/pyproject configuration. Locate the sentence near the standalone
Ruff/Ruff format/pyright guidance and revise it to refer to Ruff, Ruff format,
and ty instead.
---
Nitpick comments:
In `@nemoguardrails/actions/action_dispatcher.py`:
- Around line 120-134: The docstring for register_action is stale after the
signature change from Callable to Any. Update the Args description for action in
action_dispatcher.py so it matches the current register_action(action: Any,
name: Optional[str] = None, override: bool = True) signature and reflects that
action may be a broader object, while keeping the rest of the behavior and
action_meta/__name__ lookup in sync.
- Around line 128-134: The name resolution and validation logic is duplicated in
the action dispatcher path, the action decorator, and register_filter, so
extract a shared helper for resolving explicit names and enforcing the
string/non-empty check. Update the logic in action_dispatcher, actions, and
taskmanager to call the shared helper instead of repeating the `name or
getattr(..., "__name__", None)` plus `isinstance(..., str)`/`ValueError`
pattern, and keep the error message consistent via the helper’s kind parameter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 08f0fedd-6268-4a00-aa9b-ee1a9e401644
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
.pre-commit-config.yamlAGENTS.mdCONTRIBUTING.mdgotchas.mdnemoguardrails/actions/action_dispatcher.pynemoguardrails/actions/actions.pynemoguardrails/actions/llm/generation.pynemoguardrails/cli/chat.pynemoguardrails/cli/debugger.pynemoguardrails/cli/providers.pynemoguardrails/embeddings/cache.pynemoguardrails/embeddings/providers/azureopenai.pynemoguardrails/embeddings/providers/fastembed.pynemoguardrails/embeddings/providers/nim.pynemoguardrails/embeddings/providers/openai.pynemoguardrails/guardrails/actions/content_safety_action.pynemoguardrails/guardrails/actions/tool_result_action.pynemoguardrails/kb/kb.pynemoguardrails/llm/clients/base.pynemoguardrails/llm/taskmanager.pynemoguardrails/rails/llm/llmrails.pynemoguardrails/server/api.pynemoguardrails/server/schemas/utils.pynemoguardrails/streaming.pynemoguardrails/tracing/adapters/opentelemetry.pynemoguardrails/tracing/adapters/registry.pypyproject.tomltests/test_action_dispatcher.pytests/test_actions.pytests/test_basic_embeddings_index_numpy.pytests/test_guardrail_exceptions.pytests/tracing/adapters/test_log_adapter_registry.pytriage.md
2a8b7b2 to
ffc90ce
Compare
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
ffc90ce to
cc8b190
Compare
|
Staged Fern docs preview: https://nvidia-preview-pr-2115.docs.buildwithfern.com/nemo/guardrails |
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
cc8b190 to
4d03229
Compare
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Signed-off-by: Pouyanpi <13303554+Pouyanpi@users.noreply.github.com>
Description
Replace Pyright with ty for the scopes configured in
pyproject.toml, resolve the diagnostics found during migration, and run ty through the official pre-commit hook.Related Issue(s)
Verification
make test WORKERS=1 TEST='tests/test_action_dispatcher.py tests/test_actions.py tests/test_basic_embeddings_index_numpy.py tests/test_embeddings_providers_mock.py tests/test_guardrail_exceptions.py tests/tracing/adapters/test_log_adapter_registry.py'— 77 passeduv run --locked --group=dev pre-commit run --all-filesAI Assistance
Checklist